Add EuiDelayHide component #412

sorenlouv · 2018-02-15T16:47:49Z

Closes #382

This component will conditionally show/hide a child component. It will ensure that the child is visible for at least 1000ms (default). This avoids UI glitches with loading spinners and other elements that are rendered conditionally.

<EuiDelayHide hide={!isLoading} render={() => <div>Loading...</div>} />

EuiDelayHide uses a render props to render the child component.

TODO:

Add EuiDelayHide component
Add tests
Update changelog
Update documentation

sorenlouv · 2018-02-15T16:59:16Z

@cjcenizal I was not sure where this component belonged, so added it to src/components. Let me know if it fits better somewhere else.

cjcenizal

👏 This is awesome man! I had a few minor suggestions about the docs and some questions about the desired behavior.

cjcenizal · 2018-02-15T18:04:03Z

src-docs/src/views/delay_hide/delay_hide.js

+import { EuiFlexItem } from '../../../../src/components/flex/flex_item';
+import { EuiCheckbox } from '../../../../src/components/form/checkbox/checkbox';
+import { EuiFormRow } from '../../../../src/components/form/form_row/form_row';
+import { EuiFieldNumber } from '../../../../src/components/form/field_number';


Can we import all of these from the main components module? I have a regex which will replace that with @elastic/eui in the code example.

import { EuiDelayHide, EuiFlexItem, EuiCheckbox, EuiFormRow, EuiFieldNumber, } from '../../../../src/components';

Sure I'll do that. But it seems to give some problems to vscode (and probably static analysis tools in general). Eg. I can "go to definition" when I use
import { EuiCheckbox } from '../../../../src/components/form/checkbox/checkbox';, but not with
import { EuiDelayHide} from '../../../../src/components';.

This makes it a bit harder to navigate the codebase. Also: automatic imports will import using the first.

I hear you. Though I'm not sure we should optimize the code for any particular editor... I use SublimeText myself so I don't get the benefits vscode is giving you. In the absence of a standard editor, I think it's better to optimize for no editor, i.e. make the code as readable as possible when viewed as regular text.

cjcenizal · 2018-02-15T18:04:39Z

src-docs/src/views/delay_hide/delay_hide.js

+
+  render() {
+    return (
+      <div>


Can we import React, { Fragment } from 'react'; at the top of the file and replace this div with a <Fragment>?

cjcenizal · 2018-02-15T18:05:43Z

src-docs/src/views/delay_hide/delay_hide_example.js

+      ],
+      text: (
+        <p>
+          <EuiCode>DelayHide</EuiCode> is a component for conditionally toggling


Can we change this to <EuiCode>EuiDelayHide</EuiCode> to use the full component name?

cjcenizal · 2018-02-15T18:10:21Z

src/components/delay_hide/delay_hide.js

+    };
+
+    this.lastRenderedTime = this.props.hide ? 0 : Date.now();
+    this.shouldRender = false;


Maybe instead of setting these variables here, we should extract the logic in componentWillReceiveProps into an instance method and invoke it either here in the constructor or in componentWillMount? I think this is a corner case, but it seems the most "correct".

I'm fine with that. I'm not 100% sure how you want it to be - can you show it with code? :)

cjcenizal · 2018-02-15T18:12:24Z

src/components/delay_hide/delay_hide.js

+    clearTimeout(this.timeout);
+
+    const visibleDuration = Date.now() - this.lastRenderedTime;
+    const timeRemaining = minimumDuration - visibleDuration;


Do we want to resume the countdown instead of resetting it entirely? In other words, why not debounce the hiding action?

If I understand you correctly, I think the current implementation is correct.

In our case there might be several loading events with a few ms between them. If each takes 500ms, and they have 10ms between them, we should show the loading spinner for 1520ms (500ms + 10ms + 500ms + 10ms + 500ms) and then hide it immediately after the last loading event finishes.

If we reset the timer between each event the loading spinner will be displayed for 2020ms (500ms + 10ms + 500ms + 10 + 1000ms).

Ah, you know what, you're right. I completely forgot that the whole point of this component is to show the waiting feedback for a bare minimum amount of time.

cjcenizal · 2018-02-15T18:17:56Z

src/components/delay_hide/delay_hide.js

+  };
+
+  shouldComponentUpdate() {
+    return this.shouldRender;


Do we need to use shouldComponentUpdate here? What's it doing for us?

No - will remove it again.
I was of the impression that it would avoid re-rendering the children but it happens anyway so ¯_(ツ)_/¯

cjcenizal · 2018-02-15T18:20:45Z

src/components/delay_hide/delay_hide.test.js

+describe('when EuiDelayHide is visible initially and is changed to hidden', () => {
+  let wrapper;
+  beforeEach(() => {
+    jest.useFakeTimers();


I didn't know this existed! I'm going to use this to shave time off our other tests. Thanks!

Great! One gotcha I ran into, is that it doesn't do anything about Date.now and similar date methods. It's just setTimeout and setInterval.

cjcenizal · 2018-02-15T18:21:47Z

src/components/delay_hide/delay_hide.test.js

+    );
+  });
+
+  test('it should be hidden initially', async () => {


Should it be hidden initially? I think it should behave the same as setting hide to true, and should apply the delay. If the consumer really doesn't want it visible, they should just not render it at all.

I think it should behave the same as setting hide to true, and should apply the delay

I think if the initial value is hide=true there is no reason to display it. The whole point of this component is to avoid "UI glitching". If an element is hidden from the start, it won't need the delay.
I think this is very different from a component changing visibility from visible to hidden (which is where we want the delay).

It would also add an extra burden on the consumer. As it is now I can render the global loading spinner with a prop from redux (isLoading is a reduced value combining all loading states in the entire app):

export default ({ isLoading }) => { return ( <EuiDelayHide hide={!isLoading} render={<EuiProgress size="xs" position="fixed" />} /> ); };

If EuiDelayHide was to always render children on init I would have to keep a flag, to avoid rendering the loading spinner initially:

let isInitialLoad = true; export default ({ isLoading }) => { if (!isLoading && isInitialLoad) { return null; } isInitialLoad = false; return ( <EuiDelayHide hide={!isLoading} render={<EuiProgress size="xs" position="fixed" />} /> ); };

I think we should wait a little with this but if it becomes a requested feature we can add a flag, eg leading delay (liked lodash.debounce has it).

I think we should wait a little with this but if it becomes a request feature we can add a flag, eg leading delay (liked lodash.debounce has it).

Sounds good!

cjcenizal · 2018-02-15T18:23:39Z

src-docs/src/views/delay_hide/delay_hide.js

+            <EuiDelayHide
+              hide={this.state.hide}
+              minimumDuration={this.state.minimumDuration}
+              render={() => <div>Hello world</div>}


Can we use an instance of <EuiLoadingSpinner size="m"/> instead of the div, just to make the use case a little more realistic?

cjcenizal · 2018-02-15T18:26:14Z

Oh I also got a console error somehow. Looks like it's coming from setStateDelayed.

sorenlouv · 2018-02-15T20:51:44Z

Oh I also got a console error somehow. Looks like it's coming from setStateDelayed.

Ouch, that looks nasty. Good catch. Can you reproduce it?
There are only two setState calls - one in componentWillReceiveProps and the other in setStateDelayed. It could happen that the component gets destroyed (eg. by navigating away) and then the delayed setState call is fired. I think if I cancel the timeout in componentWillUnmount it should fix it.

sorenlouv

Thanks for all the feedback!

sorenlouv · 2018-02-16T00:50:08Z

src-docs/src/views/delay_hide/delay_hide.js

+
+  render() {
+    return (
+      <div>


sorenlouv · 2018-02-16T00:50:19Z

src-docs/src/views/delay_hide/delay_hide.js

+            <EuiDelayHide
+              hide={this.state.hide}
+              minimumDuration={this.state.minimumDuration}
+              render={() => <div>Hello world</div>}


sorenlouv · 2018-02-16T00:50:28Z

src-docs/src/views/delay_hide/delay_hide_example.js

+      ],
+      text: (
+        <p>
+          <EuiCode>DelayHide</EuiCode> is a component for conditionally toggling


sorenlouv · 2018-02-16T00:52:41Z

src/components/delay_hide/delay_hide.js

+    };
+
+    this.lastRenderedTime = this.props.hide ? 0 : Date.now();
+    this.shouldRender = false;


I'm fine with that. I'm not 100% sure how you want it to be - can you show it with code? :)

sorenlouv · 2018-02-16T01:11:09Z

src/components/delay_hide/delay_hide.js

+    clearTimeout(this.timeout);
+
+    const visibleDuration = Date.now() - this.lastRenderedTime;
+    const timeRemaining = minimumDuration - visibleDuration;


If I understand you correctly, I think the current implementation is correct.

In our case there might be several loading events with a few ms between them. If each takes 500ms, and they have 10ms between them, we should show the loading spinner for 1520ms (500ms + 10ms + 500ms + 10ms + 500ms) and then hide it immediately after the last loading event finishes.

If we reset the timer between each event the loading spinner will be displayed for 2020ms (500ms + 10ms + 500ms + 10 + 1000ms).

sorenlouv · 2018-02-16T01:12:25Z

src/components/delay_hide/delay_hide.js

+  };
+
+  shouldComponentUpdate() {
+    return this.shouldRender;


No - will remove it again.
I was of the impression that it would avoid re-rendering the children but it happens anyway so ¯_(ツ)_/¯

sorenlouv · 2018-02-16T01:13:50Z

src/components/delay_hide/delay_hide.test.js

+describe('when EuiDelayHide is visible initially and is changed to hidden', () => {
+  let wrapper;
+  beforeEach(() => {
+    jest.useFakeTimers();


Great! One gotcha I ran into, is that it doesn't do anything about Date.now and similar date methods. It's just setTimeout and setInterval.

sorenlouv · 2018-02-16T01:28:40Z

src/components/delay_hide/delay_hide.test.js

+    );
+  });
+
+  test('it should be hidden initially', async () => {


I think it should behave the same as setting hide to true, and should apply the delay

I think if the initial value is hide=true there is no reason to display it. The whole point of this component is to avoid "UI glitching". If an element is hidden from the start, it won't need the delay.
I think this is very different from a component changing visibility from visible to hidden (which is where we want the delay).

It would also add an extra burden on the consumer. As it is now I can render the global loading spinner with a prop from redux (isLoading is a reduced value combining all loading states in the entire app):

export default ({ isLoading }) => { return ( <EuiDelayHide hide={!isLoading} render={<EuiProgress size="xs" position="fixed" />} /> ); };

If EuiDelayHide was to always render children on init I would have to keep a flag, to avoid rendering the loading spinner initially:

let isInitialLoad = true; export default ({ isLoading }) => { if (!isLoading && isInitialLoad) { return null; } isInitialLoad = false; return ( <EuiDelayHide hide={!isLoading} render={<EuiProgress size="xs" position="fixed" />} /> ); };

I think we should wait a little with this but if it becomes a requested feature we can add a flag, eg leading delay (liked lodash.debounce has it).

cjcenizal

LGTM! Thanks for doing this @sqren !

sorenlouv · 2018-02-16T08:44:52Z

@cjcenizal I don't have write access. Will you merge?

epixa · 2018-02-16T15:14:25Z

@sqren I just double checked and all Elastic employees should have write access. I just gave you admin access though, just to be sure.

sorenlouv · 2018-02-16T15:19:20Z

@cjcenizal What is the process for bumping eui version? Should I do that? And what about publishing?

epixa · 2018-02-16T15:22:39Z

@sqren Sorry about that. It looks like the master branch access was locked down to only administrators for some reason. I removed that restriction so anyone with write access can contribute.

sorenlouv requested a review from cjcenizal February 15, 2018 16:47

cjcenizal suggested changes Feb 15, 2018

View reviewed changes

sorenlouv commented Feb 16, 2018

View reviewed changes

cjcenizal approved these changes Feb 16, 2018

View reviewed changes

sorenlouv added 7 commits February 16, 2018 02:56

Add EuiDelayHide component

c2a4de7

Updated changelog

5e15d35

Added docs

b5ca7ef

Minor tweak

e6511d5

Fix issue with component calling setState after unmounting

1261c8b

Address feedback

65d536d

Increased example duration to 3000

67b2a23

sorenlouv force-pushed the add-delay-hide-component branch from 283f814 to 67b2a23 Compare February 16, 2018 01:58

sorenlouv merged commit f19218f into elastic:master Feb 16, 2018

sorenlouv deleted the add-delay-hide-component branch February 16, 2018 15:18

Add EuiDelayHide component #412

Add EuiDelayHide component #412

Conversation

sorenlouv commented Feb 15, 2018 • edited Loading

sorenlouv commented Feb 15, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Feb 15, 2018

sorenlouv commented Feb 15, 2018

sorenlouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

sorenlouv commented Feb 16, 2018 • edited Loading

epixa commented Feb 16, 2018

sorenlouv commented Feb 16, 2018 • edited Loading

epixa commented Feb 16, 2018

sorenlouv commented Feb 15, 2018 •

edited

Loading

sorenlouv Feb 15, 2018 •

edited

Loading

sorenlouv Feb 16, 2018 •

edited

Loading

sorenlouv Feb 16, 2018 •

edited

Loading

sorenlouv Feb 16, 2018 •

edited

Loading

sorenlouv Feb 16, 2018 •

edited

Loading

sorenlouv commented Feb 16, 2018 •

edited

Loading

sorenlouv commented Feb 16, 2018 •

edited

Loading